feat(SQL): L1 to L3 SQL lowering via DataFusion with TDD - #4
Conversation
milindsrivastava1997
changed the title
milindsrivastava1997
left a comment
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Code Review — 15 items
Bugs
#1 — lower_limit maps missing fetch → n: 0 (semantic bug)
crates/lower/src/sql.rs, lower_limit:
n: eval_fetch(&limit.fetch).unwrap_or(0) as u64,None fetch means "no LIMIT" (unlimited), but n: 0 reads as "return zero rows." QueryExpr::Limit.n should be Option<u64>, or use u64::MAX as the sentinel. A LIMIT-less Limit plan node (emitted by DataFusion when offset > 0 with no cap) becomes a zero-row filter.
#2 — WindowFuncKind::NthValue(0) is always wrong
crates/lower/src/sql.rs, lower_window_func_kind:
"nth_value" => Ok(WindowFuncKind::NthValue(0)),The N argument lives in wf.args[1] but is never extracted. Every NTH_VALUE(col, N) silently becomes NthValue(0). No test catches this.
#3 — extract_time_range only fires when Filter directly wraps TableScan
crates/lower/src/sql.rs, lower_filter:
let inner = strip_aliases(&filter.input);
if let LogicalPlan::TableScan(scan) = inner { ...Stacked Filter → Filter → TableScan (two separate WHERE conditions) causes the outer filter to skip time-extraction entirely. Need a test for WHERE ts > 1000 AND ts < 2000 AND value > 0 AND host = 'x' to verify.
#4 — push_columns_into_scan doesn't recurse through Filter
crates/lower/src/sql.rs, push_columns_into_scan:
The function only rewrites a Scan that is the direct child of Project. When the plan is Project → Filter → Scan (any WHERE clause), Scan.columns stays empty. The "empty = all columns" fallback masks this but violates the stated semantic contract.
#5 — lower_window silently drops all but the first window expression
crates/lower/src/sql.rs, lower_window:
let first = window.window_expr.first().ok_or_else(|| ...)?;DataFusion emits one Window plan node for all functions sharing the same OVER clause. The second (and beyond) window function is dropped with no error. Should either error on window_expr.len() > 1 or loop and stack WindowFunc nodes.
#6 — UInt64 → i64 cast silently wraps on overflow
crates/lower/src/sql.rs, scalar_value_to_l3:
ScalarValue::UInt64(Some(v)) => Ok(L3Scalar::Int64(*v as i64)),Values > i64::MAX silently become negative. Use i64::try_from(*v).map_err(|_| LoweringError::InvalidExpression(...)) instead.
Design Issues
#7 — AggIntent doesn't carry the aggregated column
crates/core/src/intent_algebra/expr.rs, AggIntent:
Sum, Min, Max, Avg, Stddev have no col: ColumnRef field. Consequences: output_type for Min/Max receives a dummy Float64, so MIN(ts) wrongly reports Float64. Aggregate schema columns are named agg_{i} and carry the wrong type. Adding this field is a breaking API change — better done now before L4 exists.
#8 — expr_to_group_key / expr_to_col_ref fall back to format!("{expr}")
crates/lower/src/sql.rs:
_ => Ok(GroupKey(format!("{expr}"))),The DataFusion Expr Display format is not stable across versions. Should return Err(LoweringError::UnsupportedFeature(...)) for anything that isn't a plain column reference.
#9 — SchemaCatalog has no validation that time_column names an actual column
crates/core/src/intent_algebra/schema.rs:
TableSchema::time_column is Option<String> with no check that the named column exists in columns. A misconfigured catalog silently produces a table where time-range extraction never fires. A Result-returning constructor or a debug-mode assert would catch this early.
Test Gaps
#10 — No test for NthValue N extraction
Directly related to bug #2. Add a test: SELECT NTH_VALUE(value, 2) OVER (ORDER BY ts) FROM metrics and assert the resulting NthValue variant holds 2, not 0.
#11 — test_lower_batch_error_is_per_query only tests parse failure
crates/lower/tests/sql_lowering.rs:
The bad query "NOT VALID SQL !!!" fails at the DataFusion SQL parser, not at our lowering layer. There should also be a test where SQL parses successfully but lowering fails (e.g., SELECT array_agg(value) FROM metrics — unsupported aggregate).
#12 — TopK test doesn't verify by.len()
crates/lower/tests/sql_lowering.rs, test_order_by_desc_limit_becomes_topk:
assert!(by.iter().any(|c| c.0 == "host"));Should also assert by.len() == 1 to catch spurious extra columns in the TopK by list.
#13 — No test for stacked Filters against a time-column table
Directly related to bug #3. Add a test with two non-time predicates plus a time predicate in a single WHERE clause that DataFusion splits into nested Filter nodes, and verify the time range is still extracted.
#14 — No test for multi-window-expr Window plan
Directly related to bug #5. Add a test: SELECT SUM(value) OVER (PARTITION BY region), AVG(value) OVER (PARTITION BY region) FROM metrics and assert both window functions produce output (currently only the first is kept).
#15 — test_unknown_table_returns_error accepts either of two error variants
crates/lower/tests/sql_lowering.rs:
assert!(matches!(err, LoweringError::DataFusion(_) | LoweringError::TableNotFound(_)));This passes even if the error variant changes unexpectedly. Pin to DataFusion(_) (what actually fires when DataFusion rejects the unknown table at plan time) and add a comment explaining why.
milindsrivastava1997
left a comment
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Thorough review of the SQL L1→L3 lowering PR. The architecture is clean and the TDD approach is well-executed — the test suite covers the happy paths comprehensively. Comments below are ordered by severity.
Bugs / Correctness (items 1–4)
API / Design (items 5–9)
Code Quality (items 10–13)
Test-coverage gaps not tied to a specific line are listed as general comments:
- No test for
LIMIT n OFFSET m(offset could silently be 0) - No test for
NOT BETWEEN low AND highnormalizing toBoolOr - No test for
NOT IN (...)withnegated: true - No test for time predicate with column on the right side (
1000 < ts) - No test for
DISTINCT ON (col)(see inline comment — currently silent data loss) - No test for
TryCastvsCastproducing the same IR - No test for
populate_schemasonSetOp/Merge
| /// Returns `None` for parametric (non-literal) fetch expressions. | ||
| fn eval_fetch(expr_opt: &Option<Box<Expr>>) -> Option<usize> { | ||
| expr_opt.as_ref().and_then(|e| match e.as_ref() { | ||
| Expr::Literal(ScalarValue::Int64(Some(v))) => Some(*v as usize), |
There was a problem hiding this comment.
Bug: panic on negative LIMIT/OFFSET in debug mode.
LIMIT -1 is invalid SQL but DataFusion may still parse it. Casting a negative i64 to usize panics in debug and gives a huge value in release.
Fix:
Expr::Literal(ScalarValue::Int64(Some(v))) if *v >= 0 => Some(*v as usize),Same guard needed for the Int32 arm on line 496.
| LogicalPlan::Distinct(d) => { | ||
| let input = match d { | ||
| Distinct::All(input) => input.as_ref(), | ||
| Distinct::On(on) => on.input.as_ref(), |
There was a problem hiding this comment.
Bug: DISTINCT ON (...) silently drops its ON-column list.
on.on_expr and on.select_expr are discarded; the emitted QueryExpr::Distinct { cols: vec![] } has no ON semantics. SELECT DISTINCT ON (region) ts, value FROM metrics would be treated as a plain DISTINCT, changing the result set.
Either:
Distinct::On(_) => return Err(LoweringError::UnsupportedFeature("DISTINCT ON".into())),or populate cols from on.on_expr if the semantics are intentionally preserved.
| ScalarValue::TimestampNanosecond(Some(ns), _) => Some(*ns / 1_000_000), | ||
| ScalarValue::TimestampMicrosecond(Some(us), _) => Some(*us / 1_000), | ||
| ScalarValue::TimestampSecond(Some(s), _) => Some(*s * 1_000), | ||
| _ => None, |
There was a problem hiding this comment.
Bug (silent): float/decimal time literals fall through as non-time conjuncts.
scalar_to_ms returns None for Float64, Decimal128, etc. So WHERE ts > 1000.0 is left as a Filter predicate rather than folded into time_range, with no error or warning. Add a comment here noting the limitation, or convert common float types:
ScalarValue::Float64(Some(v)) => Some(*v as i64), // ms-since-epoch stored as floator return an InvalidExpression so callers know time extraction failed.
| // Use DataFusion's display string for each aggregate expression — this is | ||
| // exactly the name DataFusion assigns to the output column (e.g. "MIN(metrics.ts)"), | ||
| // which the enclosing Projection uses to reference aggregate outputs. | ||
| let output_names: Vec<String> = agg.aggr_expr.iter().map(|e| format!("{e}")).collect(); |
There was a problem hiding this comment.
Bug (latent): coupling to DataFusion's Display format for aggregate names.
let output_names: Vec<String> = agg.aggr_expr.iter().map(|e| format!("{e}")).collect();The Project schema arm uses these strings to match aggregate output columns by name. If DataFusion changes its Display impl for Expr (e.g., starts qualifying as metrics.MIN(ts) instead of MIN(metrics.ts)), name matching will silently fail and all aggregate fields will fall through to Float64.
At minimum: add a comment pinning the DataFusion version this was verified against. Better: assert in a test that format!("{e}") for a known expr matches the expected string, so a DataFusion upgrade breaks loudly rather than silently.
| | Self::Rate { .. } | ||
| | Self::Increase { .. } => L3DataType::Float64, | ||
| Self::TopK { .. } => { | ||
| panic!("TopK is multi-column; derive schema via QueryExpr::output_schema") |
There was a problem hiding this comment.
Design: panic! in a public library method is hostile to callers.
A downstream user calling output_type on a TopK they received from the lowerer will get an unexplained panic. Consider returning Option<L3DataType> (returning None for multi-column intents) so callers can handle it:
pub fn output_type(&self, input: &L3Field) -> Option<L3DataType> {
match self {
Self::TopK { .. } => None,
// ... other arms return Some(...)
}
}The existing call site in output_schema already guards against TopK before calling this; returning Option makes that contract explicit in the type.
| }; | ||
|
|
||
| // Guard: only SQL languages are handled by this lowerer. | ||
| if !matches!( |
There was a problem hiding this comment.
Design: two entry points map to the same DataFusion lowerer.
QueryLanguage::DataFusion (line 31) and QueryLanguage::SQL(SqlDialect::DataFusionSQL) (line 43) both pass through to SqlLowerer via separate if branches. Having two spellings of the same thing will confuse callers. A doc comment explaining why both exist (or consolidating to one canonical variant) would help.
| /// DataFusion's unoptimized plan never sets `TableScan.projection`, so this | ||
| /// compensates without requiring optimizer passes that could alter other | ||
| /// plan-node shapes our lowerer depends on. | ||
| fn push_columns_into_scan(child: QueryExpr, cols: &[ProjectItem]) -> QueryExpr { |
There was a problem hiding this comment.
Code quality: push_columns_into_scan only recurses through Filter.
The comment says it handles Project → Filter → Scan, which it does. But Project → Aggregate → Filter → Scan (e.g., SELECT region, COUNT(*) FROM metrics WHERE value > 0 GROUP BY region) does not get the Scan columns populated from the projection's column refs. This is currently benign because lower_aggregate doesn't call push_columns_into_scan, but the asymmetry is surprising and will bite anyone who extends the lowerer. A note in the doc comment about what topologies are and aren't handled would help.
| let table_name = scan.table_name.to_string(); | ||
| if let Some(schema) = self.catalog.tables.get(&table_name) { | ||
| if let Some(time_col) = &schema.time_column { | ||
| debug_assert!( |
There was a problem hiding this comment.
Code quality: debug_assert! is skipped in release builds.
If a caller constructs a SchemaCatalog with an invalid time_column name and runs in release mode, the invalid catalog silently produces wrong plans (no time-range extraction). TableSchema::validate() exists and catches this — consider calling it in SchemaCatalog's construction path (or in lower_batch before processing) to enforce the invariant in all build profiles.
| /// Split `expr` into `(time_range, non_time_conjuncts)`. | ||
| /// Time-bound conjuncts are folded into the `TimeRange`; the rest are returned | ||
| /// as a `Vec<&Expr>` so the caller can translate them with `df_expr_to_l3`. | ||
| pub(crate) fn extract_time_range<'a>( |
There was a problem hiding this comment.
Code quality: extract_time_range is pub(crate) but has no direct unit tests.
All coverage goes through the integration tests, which don't exercise every branch (e.g., column on the right side of the comparison: 1000 < ts, overlapping ranges from repeated bounds, NOT BETWEEN). Given how central this function is to correctness, a dedicated unit-test module in sql.rs (or a separate tests/time_extraction.rs) covering those edge cases would add confidence.
| /// Translate a slice of DataFusion `Expr`s (non-time conjuncts) into a single | ||
| /// `L3Expr`. A single element is returned as-is; multiple elements are wrapped | ||
| /// in `L3Expr::BoolAnd`. | ||
| fn conjuncts_to_l3expr(conjuncts: Vec<&Expr>) -> Result<L3Expr, LoweringError> { |
There was a problem hiding this comment.
Nit: parts.remove(0) is O(n) for the single-element fast path.
if parts.len() == 1 {
Ok(parts.remove(0)) // shifts remaining elements (none here, but still)
}Prefer:
if parts.len() == 1 {
Ok(parts.pop().unwrap())
}pop() is O(1) and more idiomatic for "take the only element".
| } | ||
| let child = self.lower_plan(&limit.input)?; | ||
| Ok(QueryExpr::Limit { | ||
| child: make_untyped_node(child), |
There was a problem hiding this comment.
Bug: OFFSET is silently discarded when folding Limit+Sort+Aggregate into TopK.
LIMIT k OFFSET n ORDER BY ... DESC on an aggregate will emit AggIntent::TopK and drop n. A caller expecting to page through top-k results gets the wrong slice with no error.
Fix: guard on a non-zero offset and fall through to the regular Sort + Limit path when one is present:
if let Some(k) = eval_fetch(&limit.fetch) {
if eval_fetch(&limit.skip).unwrap_or(0) == 0 { // <-- add this guard
let inner = strip_aliases(&limit.input);
...
}
}| // When the direct child is a TableScan and the table has a time column, | ||
| // split the predicate: time bounds go into Source::Table.time_range; the | ||
| // remaining non-time conjuncts become the Filter predicate. | ||
| let inner = strip_aliases(&filter.input); |
There was a problem hiding this comment.
Correctness gap: time extraction only fires when the direct child is a TableScan.
DataFusion can produce a Filter(Filter(TableScan)) topology when it stacks multiple filter conditions. In that case the outer Filter's child is another Filter, not a TableScan, so this branch is skipped. The time bounds remain as opaque predicates and Source::Table.time_range stays None even though the SQL clearly expressed a time bound.
Consider recursing through Filter children here, or add a test that explicitly asserts the current (limited) behaviour so regressions are caught if DataFusion's planning changes.
| "first_value" => Ok(WindowFuncKind::FirstValue), | ||
| "last_value" => Ok(WindowFuncKind::LastValue), | ||
| // NthValue(0) is a sentinel; lower_window extracts the real N from args. | ||
| "nth_value" => Ok(WindowFuncKind::NthValue(0)), |
There was a problem hiding this comment.
Design: NthValue(0) as a sentinel is fragile.
This emits a WindowFuncKind::NthValue(0) that must be resolved by lower_window before it enters the tree. A debug_assert! catches the bypass in debug builds, but in release the sentinel leaks silently and schema derivation will treat it as NthValue(0) (meaning the 0th row, which is not a valid SQL index).
If lower_window_func_kind is ever called from a future code path that isn't lower_window (e.g. a PromQL path that reuses this helper), the sentinel will escape undetected in production.
Consider NthValue(Option<u64>) instead: None = unresolved (panics loudly in schema derivation if accidentally used), Some(n) = valid N.
| /// topology where an Aggregate sits between the Project and the Scan. Any | ||
| /// downstream stage that uses `Scan.columns` for pruning or cost estimation | ||
| /// will see an unconstrained (full) scan in those cases. TODO: propagate | ||
| /// column refs through the Aggregate child when implementing column-pruning. |
There was a problem hiding this comment.
Known gap needs a test to document current behaviour.
The comment correctly describes the limitation (Project → Aggregate → * → Scan leaves Scan.columns empty), but there is no test asserting this. Without a test, a future fix or DataFusion upgrade could change the behaviour silently in either direction.
Add a test like:
// SELECT region, COUNT(*) FROM metrics GROUP BY region
// → Scan.columns must be empty ("all columns" semantics) because push_columns_into_scan
// does not propagate through Aggregate.
let source = find_source(&result).unwrap();
let Source::Table { columns, .. } = source else { panic!() };
assert!(columns.is_empty(), "aggregate topology: columns should be empty (all-columns scan)");This makes the gap visible in CI and signals intent for whoever implements column-pruning later.
| return entries | ||
| .iter() | ||
| .map(|_| Err(LoweringError::InvalidExpression(msg.clone()))) | ||
| .collect(); |
There was a problem hiding this comment.
Design: invalid catalog fails all queries, even those that don't touch the bad table.
If catalog.tables contains table A with a bad time_column and table B that is valid, a batch of queries against B only will still return Err(InvalidExpression) for every entry. This is surprising and could obscure the real problem in logs.
Two options:
- Fail all (current): keep it, but document the behaviour explicitly and add a test that asserts it.
- Fail selectively: validate lazily per query inside
SqlLowerer::lower, so only queries that reference the invalid table fail.
At minimum, the error message should name the offending table (it does — good) and the call site should make the batch-wide behaviour visible in tests.
| /// uses these names so that an enclosing `Project` can resolve aggregate | ||
| /// outputs by the names DataFusion assigned them (e.g. `"MIN(metrics.ts)"`). | ||
| /// Empty = fall back to synthetic `"agg_{i}"` names. | ||
| output_names: Vec<String>, |
There was a problem hiding this comment.
Design debt: output_names couples core IR to DataFusion's internal naming convention — this is a pre-L4 blocker.
The output_names field is populated with DataFusion's own schema field names (e.g. "MIN(metrics.ts)"). An enclosing Project node resolves column references against these names, so any non-DataFusion lowerer (PromQL, future dialects) that emits an Aggregate must produce DataFusion-style names or column lookup silently falls back to Float64.
The TODO comment's proposed fix (emit a Project on top of every Aggregate to rename DataFusion's names to user-visible aliases) is the right direction. Recommend creating a tracking issue before L4 is built, since the schema contract between Aggregate and its parent Project will be much harder to change once L4 rule matching depends on field names.
| /// column (wildcard / count-star); `Some(ColumnRef(...))` = real column. `agg_col` | ||
| /// would return `Option<ColumnRef>` and this method disappears entirely. | ||
| pub fn is_wildcard(&self) -> bool { | ||
| self.0 == "*" |
There was a problem hiding this comment.
Design debt: string sentinel "*" for wildcard is fragile — recommend fixing before L4 consumes AggIntent::col().
Any column whose name happens to be "*" (unusual but not rejected by the type system) will be silently misclassified as the wildcard sentinel, causing col() to return None and schema derivation to fall back to Float64 instead of the real type.
The TODO's recommendation (Option<ColumnRef> where None = wildcard / no column) is the right fix and is a small change:
// In AggIntent:
Sum { col: Option<ColumnRef> },
// In agg_col():
fn agg_col(args: &[Expr]) -> Option<ColumnRef> { ... }This propagates naturally through col() and removes the is_wildcard() method entirely. Since L4 rules will inspect AggIntent::col() to determine sketch parameters, fixing this now avoids a breaking change later.
…ntent, SQL-node schema flow Phase A of the #4⇄#5 intent_algebra reconciliation (positional IR as base). Extends the positional L3 IR so both language front ends can target it, keeping the PromQL path green (99 tests). - expr_ir: L3Expr/CompareOp extended to the SQL∪PromQL superset (Arith/Cast/InList/FunctionCall/Case/IsNull/IsNotNull + Like/ILike, keeping Regex/NotRegex). [D2] - agg_intent: Sum/Min/Max/Avg/StdDev/Variance carry col: Option<ColumnId> (None = PromQL sample value); first-class StdDev/Variance kept. [D1/D4] - query_expr: Aggregate schema derivation binds each reducer to its own input column; Project recomputes schema from its items (was passthrough); Join concatenates left+right with per-JoinKind nullability (was left-only); SetOp drops unique_keys. + unit tests for each. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reconcile #4's name-based L3 IR with #5's positional IR, keeping #5's design (positional ColumnId + Binder) as the base per the agreed direction. Conflict resolution: - intent_algebra/{expr_ir,mod,schema}.rs, sketch_algebra/expr.rs: keep #5's positional IR (expr_ir already merged to the SQL∪PromQL superset). - intent_algebra/expr.rs: stays deleted (split into query_expr/agg_intent/…). - lower/{Cargo.toml,error.rs,lib.rs}: keep PromQL-only for now. #4's DataFusion SQL front end (crates/lower/src/sql/*, schema_pass.rs) is brought in but PARKED — not declared as a module, so it doesn't compile yet. It will be re-targeted to emit relational L2 + convert_root in follow-up commits. #4's name-based tests (core/tests/{expr_ir,schema_derivation}.rs, lower/tests/sql_lowering.rs) removed — they assert the deleted name-based API; fresh positional SQL tests come with the re-target. Workspace green: 102 tests (PromQL path intact). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tional IR) Re-points #4's SQL lowerer from name-based L3 onto the unified positional IR: both PromQL and SQL now lower through the same convert_root. - sql/mod.rs: walks DataFusion's LogicalPlan and emits relational::QueryExpr (L2). TableScan → Source carrying the catalog's resolved Schema; aggregates → AggItem{AggFunc, col} (the converter binds col→ColumnId + applies accuracy); projection → the new L2 Project; filter/sort/limit/distinct/union/topk as L2. JOIN / subquery / window funcs are rejected (no L3 analytic node yet). - sql/types.rs: SqlCatalog (table → L3 Schema) + Arrow⇄L3 DataType bridges. - sql/expr.rs: reused verbatim except ColumnRef → ColumnRef::Named. - Dropped schema_pass.rs (schema flows via Binder/SourceSpec now) and sql/time.rs (time-range pushdown isn't on #5's Source::Table). - error.rs: union PromQL + SQL variants; lib.rs: lower_sql / lower_sql_batch. - Cargo: add datafusion 43 + tokio dev-dep. Fresh sql_lowering tests (positional): WHERE folds onto Scan; multi-agg GROUP BY binds SUM(bytes)→col 3 / AVG(latency)→col 2 / service→col 1; COUNT(*)→Count; COUNT(DISTINCT)→Cardinality; JOIN rejected. Workspace green: 107 tests, clippy -D warnings clean, fmt clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- intent-algebra-reconciliation.md: note the intra-repo (#4⇄#5) reconciliation is complete — both front ends lower onto one positional IR — so the ASAP⇄control_plane plan is now the unblocked next step. - sketch_algebra/expr.rs: doc comment said Logical(Rc<L3Node>); the type is Logical(Box<QueryExpr>) (L3Node no longer exists). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-introduces the analytic-window node #4 had (dropped in the re-target), now positional + with the output-name fix #4 left as a TODO. - query_expr: `WindowFuncKind` (RowNumber/Rank/DenseRank/Lag/Lead/FirstValue/ LastValue/NthValue/Sum/Avg/Count/Min/Max) + `WindowFunc { func, args, partition_by: Vec<ColumnId>, order_by, output_name, child }`. Schema derivation = child schema + one window-output column typed per func. - relational L2: name-based `WindowFunc { …, partition_by: Vec<String>, … }`; walk/leaf_source updated. - converter: resolves args / partition_by / order_by positionally against the child schema. - sql front end: `lower_window` (re-targeted to L2) + `lower_window_func_kind`; `LogicalPlan::Window` no longer rejected. `output_name` is taken from the Window plan's schema (the name an enclosing Projection references) — fixing #4's hardcoded-name TODO. NthValue's N lifted from the literal 2nd arg. One window function per node; frames not modelled (default frame assumed). Tests: ROW_NUMBER() OVER (PARTITION BY service ORDER BY bytes DESC) → WindowFunc with partition_by=[1], order_by=[Column(3) DESC], Int64 output column resolved in the root projection; SUM(bytes) OVER (…) → WindowFunc{Sum, args:[Column(3)]}. 122 tests green, clippy -D warnings + fmt clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… mark reserved nodes (L2 review #3/#4/#6) - #4: remove `AggItem.distinct` — a write-only field the converter never read, redundant with `AggFunc::CountDistinct` (COUNT(DISTINCT) sets the func; value DISTINCT is rejected up front). It encoded "distinct" a second way, consumed zero ways. - #6: `AggItem.alias` is now `Option<String>` (was a `""` sentinel), matching its sibling `L2ProjectItem.alias`. The converter maps `None → ""` for L3's `output_names` sentinel; PromQL emits `None`, SQL `Some(name)`. - #3: doc-mark the L2 nodes no front end produces as **Reserved** — `Ref`/`LetBinding` (CSE is L3), `Merge` (UNION→SetOp), L2 `Partition` (PromQL emits `Aggregate.keys`), and `PartitionKeys::Without` (rejected up front) — so the dead converter arms read as intentional, not oversights. No behavior change. Full suite green (133), clippy + fmt clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reconciles the PromQL (#5) and SQL (#4) front ends onto a single positional ColumnId + Binder L3 IR via the shared convert_root. Includes the post-merge L3/L2 canonicality cleanups (op-enum unification, positional Distinct, PromQL grouped-aggregate convergence, qualified group keys, Expr<C>). Closes #7.
Restructure the flat 19-section developer guide into the three-part structure the doc owner asked for: Part 1 - Code Architecture, Part 2 - Interfaces and Definitions, Part 3 - How to Add X, Y, Z (each ending in how to verify). Content is moved, not rewritten: Part 1 (Mental model first, per doc-owner follow-up, then a new whole-PR architecture diagram, then "How the current pieces fit together"): - old #1 Mental model -> Part 1 #1 - new: whole-PR architecture diagram (TargetSubDAG's two entry points through ReplacementStrategy, PlanSpace/cost_sorted, explanation.rs, to a downstream consumer) -> Part 1 #2 - old #3 How the current pieces fit together -> Part 1 #3 Part 2: - old Terminology's "Implementation" definition merged into the Glossary as one more entry (### Implementation), next to ReplacementStrategy - old #2 Glossary -> Part 2 #1 (plus the merged Implementation entry and old #10 Matcher, retitled to match glossary-entry style) - old #10 Matcher (implementation.rs) -> ### Matcher inside the Glossary; implementation.rs no longer exists, so the stale title is fixed - old #19's definitional content (ReplacementExplanation/ ExplanationKind shapes, node_hash, why there's no ExplanationRule trait, location-text ownership) -> Part 2 #2 Part 3: - old #4, #5, #6, #7, #13, #14 -> Part 3 #1, Adding a new ReplacementStrategy (ending in Testing a new strategy) - old #8, #9, #15 -> Part 3 #2, Adding or customizing a CostModel (ending in Testing a new cost model) - old #12 -> Part 3 #3, Adding a new sketch algorithm, with its stale implementation.rs/binder references fixed to replacement.rs/ construct_summary vocabulary, plus a new "Verifying a new sketch algorithm" close grounded in the existing coverage-matrix tests - old #11, #16, #17, #18 -> Part 3 #4-#7 (capstone + closing reference material); #18's extension-map table's implementation.rs row fixed to replacement.rs - old #19's "Using it"/"Adding a new kind" content -> Part 3 #8, Using and extending explanation.rs cargo build --workspace --all-targets is clean (docs-only change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rt reorg The reorganization into Part 1/2/3 moved every original section but deliberately left un-enumerated internal cross-references (§8, §11, §18, §19, #4-adding-..., etc.) untouched per its own 'reorganize, don't rewrite' scope. Sweep those now that the structure is settled: every old flat-numbered '§N' reference becomes 'Part X §Y' pointing at the section's actual new location, one genuinely self-referential '(§3 above)' is dropped (the text is already inside that section), and the one remaining 'bind.rs' source-file mention in the intro (bind.rs was deleted this branch) is corrected to explanation.rs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
No description provided.